-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cargo lib bin split #4
Cargo lib bin split #4
Conversation
Hi @dsheets! Making both a lib crate and a bin crate is something I had in mind since I started this project. So your MR is more than welcome! Thank you for taking the time to do it. I'll have a look to your code this week and let you know if I can merge it as is or if some modifications are needed. |
Key-value pairs were being POSTed (correct by Spotify's spec) for action=addUser but they were in the query string rather than in the x-www-form-urlencoded body. This caused librespot-java 1.6.3 to refuse connection as it could not find the action type (addUser) because it wasn't in the body. The official Spotify client uses the body of the POST. The rust librespot application must be lenient in this regard. The added urlencoding dependency was already a transitive dependency of the minreq create with 'urlencoding' feature.
Great, thanks! I fixed a bug I found when trying to connect to librespot-java and I started extracting more functionality from the binary into the library. |
185fe2c
to
656fde7
Compare
Also adds log and env_logger dependencies so the broken out function can report its status.
Also clean up a few dependencies that the binary had that were transitive dependencies only
Also make the type serde Serializable because one may wish to store device info to examine later
d881596
to
8781674
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow! You did an incredible work. Not only you separate the project in a lib and a bin crates, but you also add proper error handling, cleaned/updated dependencies and added some new features. Thanks a lot for this huge contribution!
I tested it on my local network and it still works as previously.
I only have a change request about the clientID
. I can not test it because I don't have an official Spotify hardware, but I'm pretty sure it is important. Once corrected, I will merge your contribution.
The small question about activeUser
is not blocking. I am just curious.
Merged 🎉 Once again, thank you for this major contribution. Good luck with your Christmas presents! |
Hi! I wrote this exact module a few years ago in python for a Christmas present but I'm now rewriting that present (in rust because python makes me sad) to integrate more features for another Christmas present and I came across this repo. Thanks so much for publishing it! You're saving me hours of time re-implementing.
Anyway, I would really like to have the executable and the library separate so I've split them. It builds now and the tests pass and clippy is silent.
I'm happy to collaborate to get this merged. Thanks!